Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): remove hardcoded style from ngx for toolbar component #6396

Merged
merged 5 commits into from
Sep 1, 2021
Merged

fix(core): remove hardcoded style from ngx for toolbar component #6396

merged 5 commits into from
Sep 1, 2021

Conversation

manu-kr
Copy link
Contributor

@manu-kr manu-kr commented Aug 27, 2021 •
edited
Loading

Please provide a link to the associated issue.

Part of #5025

Please provide a brief summary of this pull request.

Remove style from ngx lib as part of style cleanup for toolbar component.

Breaking Changes:

  • Added New Directory for toolbar over flow as part of style cleanup
    fd-toolbar-overflow-button, fd-toolbar-overflow-button-menu

Before

Toolbar overflow button

<button fd-toolbar-item fd-button label="Button" [compact]="true"></button>

After

Toolbar overflow button

<button fd-toolbar-item fd-button fd-toolbar-overflow-button label="Button" [compact]="true"></button>

Before

Toolbar overflow menu button

<button fd-toolbar-item fd-button label="Button" [fdMenu]="true" [compact]="true"></button>

After

Toolbar overflow menu button

<button fd-toolbar-item fd-button fd-toolbar-overflow-button fd-toolbar-overflow-button-menu label="Button" [fdMenu]="true" [compact]="true"></button>

Before

Screenshot 2021-08-27 at 1 39 49 PM

After

Screenshot 2021-08-27 at 1 40 51 PM

Please check whether the PR fulfills the following requirements

Documentation checklist:

@netlify
Copy link

netlify bot commented Aug 27, 2021 •
edited
Loading

✔️ Deploy Preview for fundamental-ngx ready!

🔨 Explore the source changes: b397803

🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-ngx/deploys/612e53e8d428440007651f2f

😎 Browse the preview: https://deploy-preview-6396--fundamental-ngx.netlify.app

@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2021

This pull request fixes 2 alerts when merging 0ae4bfa into 52d8f88 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@github-actions
Copy link

This pull request is stale because it has been open 2 days with no activity. Remove stale label or comment or this will be closed in 3 days

@github-actions github-actions bot added the stale label Aug 30, 2021
@manu-kr manu-kr requested review from N1XUS and a team August 30, 2021 04:47
@manu-kr manu-kr self-assigned this Aug 30, 2021
@manu-kr manu-kr added css clean up core Core library specific issues labels Aug 30, 2021
@manu-kr manu-kr mentioned this pull request Aug 30, 2021
Closed
21 tasks
N1XUS
N1XUS approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@N1XUS N1XUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix @Smiranin comments

@manu-kr manu-kr requested a review from Smiranin August 30, 2021 12:28
@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2021

This pull request fixes 2 alerts when merging 68dd280 into fe8a11f - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2021

This pull request fixes 2 alerts when merging a76dd60 into bac63c1 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@manu-kr manu-kr removed the stale label Aug 30, 2021
@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2021

This pull request fixes 2 alerts when merging 259828e into c0a1e58 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@manu-kr manu-kr added the ready for qa Testing should be started for the PR label Aug 30, 2021
InnaAtanasova
InnaAtanasova requested changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Breaking changes examples serve no purpose to the user. They don't need screenshots of how the component looked before and now, they need code examples of what was changes so they can modify their code as fast/easily as possible.

apps/docs/src/app/core/component-docs/toolbar/examples/toolbar-overflow-example.component.html Outdated Show resolved Hide resolved
apps/docs/src/app/core/component-docs/toolbar/examples/toolbar-overflow-example.component.html Outdated Show resolved Hide resolved
apps/docs/src/app/core/component-docs/toolbar/toolbar-docs.component.html Outdated Show resolved Hide resolved
@manu-kr
Copy link
Contributor Author

manu-kr commented Aug 31, 2021

The Breaking changes examples serve no purpose to the user. They don't need screenshots of how the component looked before and now, they need code examples of what was changes so they can modify their code as fast/easily as possible.

Updated breaking changes.

@manu-kr manu-kr requested review from InnaAtanasova and N1XUS August 31, 2021 07:40
@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2021

This pull request fixes 2 alerts when merging 6f68c0f into c0a1e58 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2021

This pull request fixes 2 alerts when merging 19fca8e into c0a1e58 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@puru-hk puru-hk added QA Approved and removed ready for qa Testing should be started for the PR labels Aug 31, 2021
@puru-hk
Copy link
Contributor

puru-hk commented Aug 31, 2021

Looks fine

@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2021

This pull request fixes 2 alerts when merging b397803 into ad03ec6 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

@DeepakSap14 DeepakSap14 mentioned this pull request Sep 1, 2021
Merged
6 tasks
@dimamarksman
Copy link
Contributor

@manu-kr shouldn't we put Breaking Changes in the PR's description?

@manu-kr
Copy link
Contributor Author

manu-kr commented Sep 1, 2021

@manu-kr shouldn't we put Breaking Changes in the PR's description?

@dimamarksman As of now we not added Breaking Changes in description, Now i added it PR description as well.

@dimamarksman dimamarksman merged commit 68ae34a into main Sep 1, 2021
@dimamarksman dimamarksman deleted the fix/toolbar branch September 1, 2021 10:35
Betrozov added a commit that referenced this pull request Sep 1, 2021
@Betrozov
Revert "fix(core): remove hardcoded style from ngx for toolbar compon…
b029db6 
…ent (#6396)"

This reverts commit 68ae34a.
@Betrozov Betrozov mentioned this pull request Sep 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InnaAtanasova InnaAtanasova InnaAtanasova approved these changes

@puru-hk puru-hk puru-hk approved these changes

@Smiranin Smiranin Smiranin approved these changes

@Betrozov Betrozov Betrozov approved these changes

@mikerodonnell89 mikerodonnell89 mikerodonnell89 approved these changes

@N1XUS N1XUS Awaiting requested review from N1XUS

Assignees

@manu-kr manu-kr

Labels
core Core library specific issues QA Approved ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@manu-kr @puru-hk @dimamarksman @mikerodonnell89 @N1XUS @Betrozov @Smiranin @InnaAtanasova